Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter] internal/persistent_queue::OnProcessingFinished is changed to a class function instead of a callback #11338

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

sfc-gh-sili
Copy link
Contributor

@sfc-gh-sili sfc-gh-sili commented Oct 3, 2024

Description

Why this change?
Each request from the queue contains multiple items, and those items could be merge-split into multiple batches when they are sent out (see #8122 for more about exporter batcher). We would like to book-keep those cases, and only call onProcessingFinished when all such batches has gone out. In this PR, onProcessingFinished is changed from a callback to a method function because it is easier to book keep index instead of functions.

Link to tracking issue

#8122
#10368

Testing

exporter/internal/queue/persistent_queue_test.go

Documentation

This is an internal change invisible to the users.

@sfc-gh-sili sfc-gh-sili marked this pull request as ready for review October 3, 2024 11:27
@sfc-gh-sili sfc-gh-sili requested a review from a team as a code owner October 3, 2024 11:27
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.79%. Comparing base (c3f09f4) to head (f3adc67).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
exporter/internal/queue/persistent_queue.go 72.00% 4 Missing and 3 partials ⚠️
exporter/internal/queue/bounded_memory_queue.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11338      +/-   ##
==========================================
- Coverage   91.80%   91.79%   -0.01%     
==========================================
  Files         432      432              
  Lines       20423    20426       +3     
==========================================
+ Hits        18749    18751       +2     
- Misses       1300     1301       +1     
  Partials      374      374              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmitryax dmitryax added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 4, 2024
Comment on lines 307 to 308
// getNextItem pulls the next available item from the persistent storage along with a callback function that should be
// called after the item is processed to clean up the storage. If no new item is available, returns false.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

exporter/internal/queue/persistent_queue.go Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

dmitryax commented Oct 5, 2024

Failing contrib tests are unrelated, broken on main

@dmitryax dmitryax merged commit fed6dfe into open-telemetry:main Oct 5, 2024
47 of 49 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 5, 2024
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this pull request Oct 8, 2024
… to a class function instead of a callback (open-telemetry#11338)

#### Description

Why this change?
Each request from the queue contains multiple items, and those items
could be merge-split into multiple batches when they are sent out (see
open-telemetry#8122
for more about exporter batcher). We would like to book-keep those
cases, and only call `onProcessingFinished` when all such batches has
gone out. In this PR, `onProcessingFinished` is changed from a callback
to a method function because it is easier to book keep index instead of
functions.

#### Link to tracking issue
open-telemetry#8122
open-telemetry#10368

#### Testing
`exporter/internal/queue/persistent_queue_test.go`

#### Documentation

This is an internal change invisible to the users.

---------

Co-authored-by: Dmitrii Anoshin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants